Add batch eth_call + pure Zig DEX math (V2/V3/Router)#42
Merged
Conversation
Batch RPC (#11): - HttpTransport.requestBatch() for JSON-RPC batch requests - BatchCaller with addCall/execute/reset and per-call error handling - Response matching by id, partial failure support DEX Math (#16): - dex/v2.zig: getAmountOut/In with configurable fees, multi-hop, arb profit - dex/v3.zig: TickMath, SqrtPriceMath, SwapMath, multi-tick simulation - dex/router.zig: mixed V2/V3 routing, binary search arb detection - uint256.zig: mulDivRoundingUp, getAmountOut delegates to dex/v2 Benchmarks: V2 25ns, V3 swap step 1.5us, 3-hop 88ns Closes #11, closes #16
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds pure-Zig Uniswap V2/V3 math and router, JSON-RPC eth_call batching (BatchCaller + HTTP batch transport), new u256 helper and benchmarks, and documentation pages for batch-calls and dex-math. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant BatchCaller
participant HttpTransport
participant RPC as "RPC Node"
Client->>BatchCaller: init(allocator, provider)
Client->>BatchCaller: addCall(to_a, data_a) -> id0
Client->>BatchCaller: addCall(to_b, data_b) -> id1
Client->>BatchCaller: addCall(to_c, data_c) -> id2
Client->>BatchCaller: execute()
BatchCaller->>HttpTransport: requestBatch([body0,body1,body2])
HttpTransport->>HttpTransport: buildBatchBody(bodies)
HttpTransport->>RPC: POST [req0, req1, req2]
RPC-->>HttpTransport: [res0, res1, res2]
HttpTransport-->>BatchCaller: raw_response_bytes
BatchCaller->>BatchCaller: parseBatchResponse(raw, [id0,id1,id2])
BatchCaller-->>Client: [BatchCallResult0, BatchCallResult1, BatchCallResult2]
sequenceDiagram
actor Searcher
participant Router
participant V2 as "dex_v2"
participant V3 as "dex_v3"
Searcher->>Router: quoteExactInput(amount_in, hops)
loop each hop
Router->>Router: quotePool(amount, pool)
alt pool is V2
Router->>V2: getAmountOut(amount, reserve_in, reserve_out, fee)
V2-->>Router: amount_out
else pool is V3
Router->>V3: simulateSwap(sqrt_price, liquidity, ticks, amount_in, zero_for_one, fee)
V3-->>Router: SwapResult{amount_out,...}
end
Router->>Router: update amount for next hop
end
Router-->>Searcher: final_amount_out
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/provider.zig (3)
467-475: SimplifyfreeBatchResultsand avoid@constCast.Two concerns:
- Line 470: The
data.len > 0check is unnecessary—allocator.freehandles zero-length slices correctly.- Line 471:
@constCastis a code smell. Sincemessageis allocated by this module, consider making the fieldmessage: []u8(mutable) inRpcErrorDatato avoid the cast.♻️ Proposed fix for freeBatchResults
pub fn freeBatchResults(allocator: std.mem.Allocator, results: []BatchCallResult) void { for (results) |r| { switch (r) { - .success => |data| if (data.len > 0) allocator.free(data), - .rpc_error => |e| if (e.message.len > 0) allocator.free(`@constCast`(e.message)), + .success => |data| allocator.free(data), + .rpc_error => |e| allocator.free(e.message), } } allocator.free(results); }And update
RpcErrorData:pub const RpcErrorData = struct { code: i64, - message: []const u8, + message: []u8, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/provider.zig` around lines 467 - 475, Remove the unnecessary length checks and the `@constCast` in freeBatchResults: change RpcErrorData.message from []const u8 to []u8 so it is freed directly, then in freeBatchResults call allocator.free(data) for .success and allocator.free(e.message) for .rpc_error without checking .len or casting; finally free the results slice as before. Ensure any places constructing RpcErrorData allocate a mutable []u8 (not []const u8) to match the new field type.
344-349: Consider documenting calldata ownership semantics.
addCallstores a reference todata: []const u8without copying. If the caller frees or modifies the calldata betweenaddCallandexecute, undefined behavior could occur. Consider either:
- Documenting that calldata must remain valid until
executecompletes, or- Copying the calldata internally (with associated memory cost).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/provider.zig` around lines 344 - 349, addCall currently stores the caller-provided slice reference (data: []const u8) into BatchCaller.calldata without copying, which can lead to UB if the original buffer is mutated/freed before execute; either explicitly document the ownership contract on addCall/BatchCaller/execute (that calldata must remain valid until execute returns) or change addCall to allocate and copy the slice into BatchCaller.calldata using the BatchCaller.allocator so execute owns stable memory; reference the addCall function, the BatchCaller type, the calldata field, and the execute function when making the change or adding the doc comment.
394-465: Consider handling missing responses more explicitly.Line 398 initializes all results to a sentinel error (
code = -1, message = ""), which is returned if a response ID doesn't match any request. While this handles the case, users won't have a clear indication that a response was missing vs. an actual RPC error.Consider either:
- Returning an error if any expected ID is missing from the response, or
- Using a dedicated error variant like
.missing_responseto distinguish from actual RPC errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/provider.zig` around lines 394 - 465, The parseBatchResponse function currently leaves entries set to a sentinel RPC error (code = -1, message = "") when an expected ID is not found; update parseBatchResponse to detect those sentinels after parsing and handle them explicitly: either return a distinct error (e.g., return error.MissingResponse) if any results entry still has the sentinel, or add a new variant to BatchCallResult (e.g., .missing_response) and set that variant for those indices so callers can distinguish missing responses from real RPC errors; implement this by iterating over results after the JSON loop, checking for the sentinel (code == -1 && message == "") and then either returning error.MissingResponse or replacing the sentinel with .missing_response (ensure the allocator ownership is correct when mutating results and free/avoid duping strings accordingly).src/uint256.zig (1)
634-659: Remove redundant denominator check.Line 637 checks
if (denominator == 0) return null;but this is unreachable—mulDivon line 636 already returnsnullwhen the denominator is zero, so if we reach line 637, denominator is guaranteed non-zero.♻️ Proposed fix
pub fn mulDivRoundingUp(a: u256, b: u256, denominator: u256) ?u256 { const result = mulDiv(a, b, denominator) orelse return null; - if (denominator == 0) return null; // Check remainder: if result * denominator != a * b, round up const a_limbs = u256ToLimbs(a);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/uint256.zig` around lines 634 - 659, The check "if (denominator == 0) return null;" inside mulDivRoundingUp is redundant because mulDiv(a, b, denominator) already returns null for a zero denominator; remove that line from the mulDivRoundingUp function (in src/uint256.zig) so control flow relies on mulDiv's null return, keeping the rest of the logic (u256ToLimbs, mulWide, remainder comparison, MAX check and result+1) unchanged.docs/content/docs/dex-math.mdx (2)
48-50: Minor: Multi-hop example reserve values may confuse users about token decimals.The example shows
USDC -> DAIwithreserve_out = 50e18, implying DAI has 18 decimals (correct), but the reserve is labeled in the comment as "ETH -> USDC" then "USDC -> DAI". The50e18for DAI reserves is fine, but the comment on line 49 says "USDC -> DAI" while using values that don't clearly demonstrate the 6-decimal (USDC) to 18-decimal (DAI) conversion users would need to handle in practice.Consider adding a brief comment clarifying decimal handling between tokens with different precisions, as this is a common source of bugs in DEX integrations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/dex-math.mdx` around lines 48 - 50, Update the multi-hop example to clarify token decimal handling: next to the reserves array (the two entries with .reserve_in/.reserve_out for the ETH->USDC and USDC->DAI hops) add a brief inline comment stating that USDC uses 6 decimals (e.g., amounts like 300_000e6 represent 300,000 USDC) while DAI/ETH use 18 decimals (e.g., 50e18 represents 50 DAI), and note that you must scale amounts when converting between tokens with different precisions; keep the numeric values unchanged but add this clarification so readers understand the unit scaling between USDC and DAI.
77-84: Consider documenting null handling for tick conversion functions.The examples use
.?to unwrap optionals fromgetSqrtRatioAtTickwithout explaining when these functions returnnull. Users should know thatnullis returned for out-of-range ticks (outside MIN_TICK/MAX_TICK bounds). A brief note would help prevent runtime panics from unexpected unwraps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/dex-math.mdx` around lines 77 - 84, The examples call getSqrtRatioAtTick and getTickAtSqrtRatio with .? unwraps but don’t document null returns; update the docs to state these functions return null for out-of-range ticks (outside MIN_TICK/MAX_TICK) and show/mention safe handling (check for null or branch before unwrapping) so users avoid runtime panics when using eth.dex_v3.getSqrtRatioAtTick(...) and eth.dex_v3.getTickAtSqrtRatio(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/dex/router.zig`:
- Around line 83-116: The probe and binary search can skip valid inputs: change
the initial probe so small_amount is not a hardcoded 1000 but e.g. small_amount
= (max_input > 0) ? std.math.min(u256(1), max_input) or at least min(max_input,
1000) and call quoteExactInput only if max_input > 0 (return null when max_input
== 0); adjust the binary search to avoid losing the upper endpoint by using a
loop condition like while (lo + 1 < hi) and computing mid = lo + (hi - lo) / 2
(remove the mid == lo break), and after the loop evaluate both candidates lo and
hi using quoteExactInput and pick the best as optimal (use quoteExactInput and
compare outputs), referencing small_amount, max_input, quoteExactInput, lo, hi,
mid, and optimal in your changes.
In `@src/dex/v2.zig`:
- Around line 28-43: Check and reject invalid pool/fee inputs before computing
V2 formulas: in getAmountOut (and likewise in getAmountIn) validate that
reserve_in != 0, reserve_out != 0, and fee_denominator != 0 (and optionally that
fee_numerator <= fee_denominator) and return 0 (or otherwise fail early) when
any of these conditions are not met so the function does not produce misleading
full-reserve or panicking results; add these guards at the start of getAmountOut
and getAmountIn using the existing parameter names (amount_in, reserve_in,
reserve_out, fee_numerator, fee_denominator) so downstream router/multi-hop
logic treats bad hops as invalid.
- Around line 119-127: The test "getAmountOut matches legacy" is tautological
because uint256_mod.getAmountOut now delegates to src/dex/v2.zig.getAmountOut
with the same fee params; replace the dynamic legacy_result call with a
hardcoded expected numeric value for the given inputs and assert v2_result
equals that constant instead (locate the test and the getAmountOut calls:
getAmountOut(amount_in, reserve_in, reserve_out, 997, 1000) and
uint256_mod.getAmountOut and remove/replace the latter with the known expected
output for these params).
In `@src/dex/v3.zig`:
- Around line 259-291: Add upfront guards in the public helpers to avoid
divide-by-zero / zero-price results: in getNextSqrtPriceFromAmount0RoundingUp
check if liquidity == 0 or sqrt_price_x96 == 0 and return null (keeping the
existing early return for amount == 0); do the same in
getNextSqrtPriceFromAmount1RoundingDown (the symmetric helper referenced around
297-317). This ensures both functions immediately return null for zero liquidity
or zero sqrt_price_x96 before any divisions or multiplications that can produce
incorrect zero/exceptional results.
- Around line 487-505: simulateSwap drops remaining input when ticks.len == 0 or
when the loop finishes with amount_remaining > 0 and active liquidity; after the
for (ticks) |tick_info| loop (and similarly at the other location around lines
539-544) add a terminal-step branch: if amount_remaining > 0 and
current_liquidity != 0, call computeSwapStep one more time using the appropriate
terminal sqrt-price (the min or max sentinel sqrt price for the swap direction)
in place of tick_info.sqrt_price_x96, then deduct consumed (step.amount_in +
step.fee_amount) from amount_remaining (or zero it), and add step.amount_out to
total_amount_out; reference functions/vars: simulateSwap, ticks,
computeSwapStep, current_sqrt_price, current_liquidity, amount_remaining,
total_amount_out to locate where to apply this final unbounded swap step.
- Around line 355-443: The function computeSwapStep uses 1_000_000 - `@as`(u256,
fee_pips) in fee math but never verifies fee_pips, allowing values > 1_000_000
to corrupt calculations; add an early guard in computeSwapStep (e.g.,
assert(fee_pips <= 1_000_000) or an explicit error return) at the top of the
function to fail fast when fee_pips is out of range, and ensure all subsequent
uses (mulDiv, mulDivRoundingUp) occur only after this validation.
- Around line 509-536: The loop that handles tick boundary updates (the branch
comparing current_sqrt_price == tick_info.sqrt_price_x96) must stop advancing
once current_liquidity reaches zero to avoid further calls to computeSwapStep
and crossing zero-liquidity gaps; after updating current_liquidity in both the
zero_for_one and else branches (where you adjust by tick_info.liquidity_net),
add a guard that checks if current_liquidity == 0 and, if so, break out of the
walk (or set the loop’s termination condition) so no further ticks are crossed
or computeSwapStep is invoked; refer to current_liquidity,
tick_info.liquidity_net, zero_for_one, computeSwapStep, and ticks_crossed to
locate where to insert this short-circuit.
In `@src/http_transport.zig`:
- Around line 64-92: The HTTP fetch code uses the wrong API: replace the
Allocating writer with a std.ArrayList(u8) and pass it via the client.fetch
.response_storage field (dynamic = &response_body) rather than .response_writer;
specifically, in requestBatch (and the same pattern in request), change var
response_body: std.Io.Writer.Allocating = .init(self.allocator) to var
response_body = std.ArrayList(u8).init(self.allocator), keep errdefer/deinit,
call client.fetch with .response_storage = .{ .dynamic = &response_body }, then
check res.status and return response_body.toOwnedSlice() (or return appropriate
errors) instead of using the nonexistent writer APIs.
---
Nitpick comments:
In `@docs/content/docs/dex-math.mdx`:
- Around line 48-50: Update the multi-hop example to clarify token decimal
handling: next to the reserves array (the two entries with
.reserve_in/.reserve_out for the ETH->USDC and USDC->DAI hops) add a brief
inline comment stating that USDC uses 6 decimals (e.g., amounts like 300_000e6
represent 300,000 USDC) while DAI/ETH use 18 decimals (e.g., 50e18 represents 50
DAI), and note that you must scale amounts when converting between tokens with
different precisions; keep the numeric values unchanged but add this
clarification so readers understand the unit scaling between USDC and DAI.
- Around line 77-84: The examples call getSqrtRatioAtTick and getTickAtSqrtRatio
with .? unwraps but don’t document null returns; update the docs to state these
functions return null for out-of-range ticks (outside MIN_TICK/MAX_TICK) and
show/mention safe handling (check for null or branch before unwrapping) so users
avoid runtime panics when using eth.dex_v3.getSqrtRatioAtTick(...) and
eth.dex_v3.getTickAtSqrtRatio(...).
In `@src/provider.zig`:
- Around line 467-475: Remove the unnecessary length checks and the `@constCast`
in freeBatchResults: change RpcErrorData.message from []const u8 to []u8 so it
is freed directly, then in freeBatchResults call allocator.free(data) for
.success and allocator.free(e.message) for .rpc_error without checking .len or
casting; finally free the results slice as before. Ensure any places
constructing RpcErrorData allocate a mutable []u8 (not []const u8) to match the
new field type.
- Around line 344-349: addCall currently stores the caller-provided slice
reference (data: []const u8) into BatchCaller.calldata without copying, which
can lead to UB if the original buffer is mutated/freed before execute; either
explicitly document the ownership contract on addCall/BatchCaller/execute (that
calldata must remain valid until execute returns) or change addCall to allocate
and copy the slice into BatchCaller.calldata using the BatchCaller.allocator so
execute owns stable memory; reference the addCall function, the BatchCaller
type, the calldata field, and the execute function when making the change or
adding the doc comment.
- Around line 394-465: The parseBatchResponse function currently leaves entries
set to a sentinel RPC error (code = -1, message = "") when an expected ID is not
found; update parseBatchResponse to detect those sentinels after parsing and
handle them explicitly: either return a distinct error (e.g., return
error.MissingResponse) if any results entry still has the sentinel, or add a new
variant to BatchCallResult (e.g., .missing_response) and set that variant for
those indices so callers can distinguish missing responses from real RPC errors;
implement this by iterating over results after the JSON loop, checking for the
sentinel (code == -1 && message == "") and then either returning
error.MissingResponse or replacing the sentinel with .missing_response (ensure
the allocator ownership is correct when mutating results and free/avoid duping
strings accordingly).
In `@src/uint256.zig`:
- Around line 634-659: The check "if (denominator == 0) return null;" inside
mulDivRoundingUp is redundant because mulDiv(a, b, denominator) already returns
null for a zero denominator; remove that line from the mulDivRoundingUp function
(in src/uint256.zig) so control flow relies on mulDiv's null return, keeping the
rest of the logic (u256ToLimbs, mulWide, remainder comparison, MAX check and
result+1) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1ae4049-9db0-4803-998b-05979dd7ae0b
📒 Files selected for processing (11)
bench/u256_bench.zigdocs/content/docs/batch-calls.mdxdocs/content/docs/dex-math.mdxdocs/content/docs/meta.jsonsrc/dex/router.zigsrc/dex/v2.zigsrc/dex/v3.zigsrc/http_transport.zigsrc/provider.zigsrc/root.zigsrc/uint256.zig
- v2: add zero-reserve/fee_denominator guards in getAmountOut/getAmountIn - v2: replace tautological legacy test with hardcoded expected value - v3: add liquidity/sqrt_price guards in getNextSqrtPrice helpers - v3: validate fee_pips <= 1M in computeSwapStep - v3: add terminal swap step in simulateSwap for remaining input - v3: break tick loop when current_liquidity reaches zero - router: fix findArbOpportunity for max_input < 1000 and max_input == 0 - router: evaluate both binary search endpoints for optimal result - uint256: remove redundant denominator==0 check in mulDivRoundingUp - provider: add doc comments for addCall borrowing and sentinel contract - docs: clarify token decimals and null returns in dex-math.mdx
- Skip negative id values in batch response parsing instead of panicking - Always free zero-length success data (was leaking empty eth_call results)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/dex/router.zig (1)
77-80: Document the concavity assumption limitation for V3 paths.The comment mentions "profit is concave for constant-product AMMs" but
Poolalso supports V3 hops. V3's concentrated liquidity with tick boundaries can create non-monotonic marginal output, so this binary search may miss the global optimum on mixed V2/V3 paths.Consider adding a note that optimal input finding is only guaranteed for all-V2 paths.
📝 Suggested doc enhancement
/// Find the optimal input amount for a circular arb path using binary search. -/// Profit is concave for constant-product AMMs, so binary search on the derivative works. +/// Profit is concave for constant-product AMMs, so binary search on the derivative works. +/// Note: For paths containing V3 pools, the profit function may not be strictly concave +/// due to liquidity discontinuities at tick boundaries. Results are best-effort. /// Returns null if no profitable opportunity exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dex/router.zig` around lines 77 - 80, The current doc for findArbOpportunity incorrectly implies the binary-search concavity guarantee holds for all Pool hops; update the comment for pub fn findArbOpportunity(hops: []const Pool, max_input: u256) ?ArbOpportunity to note that the concavity/marginal-output monotonicity assumption only holds for constant-product (V2) AMMs and is not guaranteed for V3 concentrated-liquidity hops, so the binary-search optimizer is only guaranteed to find the global optimum for all-V2 paths; also indicate expected behavior for mixed or V3-only paths (e.g., return null, require an explicit check that all Pool entries are V2 before using binary search, or fall back to sampling/global search) and reference Pool and ArbOpportunity in the comment.src/dex/v3.zig (1)
495-546: Consider adding zero-liquidity guard at loop start.The zero-liquidity break at line 544 only triggers when we've hit a tick boundary. If liquidity reaches zero mid-range (rare but possible with the current subtraction logic at lines 524-527), the loop continues but
computeSwapStepwill produce zero output, potentially causing an infinite stall on large inputs.🛡️ Optional: Add early guard at loop iteration start
for (ticks) |tick_info| { if (amount_remaining == 0) break; + if (current_liquidity == 0) break; const step = computeSwapStep(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dex/v3.zig` around lines 495 - 546, The loop over ticks can stall if current_liquidity becomes zero mid-iteration; add an early guard at the top of the for (ticks) |tick_info| loop to break if current_liquidity == 0 (or if amount_remaining == 0 already handled) before calling computeSwapStep. Locate the loop that uses computeSwapStep, check current_liquidity at its start and exit the loop immediately when it's zero so computeSwapStep is never invoked with zero liquidity and the loop cannot hang.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/provider.zig`:
- Around line 424-427: The current switch on id_val uses `@intCast` on an i64
which will panic for negative ids; in the block handling .integer (referencing
id_val and the id: u64 variable and the `@intCast` call) add a guard to ensure the
parsed integer is non-negative (and optionally <= `@intCast`(u64, `@maxValue`(u64)))
before casting, and if it fails the guard skip/continue (or handle the error)
instead of calling `@intCast` directly; this prevents panics from negative or
out-of-range JSON-RPC "id" values.
---
Nitpick comments:
In `@src/dex/router.zig`:
- Around line 77-80: The current doc for findArbOpportunity incorrectly implies
the binary-search concavity guarantee holds for all Pool hops; update the
comment for pub fn findArbOpportunity(hops: []const Pool, max_input: u256)
?ArbOpportunity to note that the concavity/marginal-output monotonicity
assumption only holds for constant-product (V2) AMMs and is not guaranteed for
V3 concentrated-liquidity hops, so the binary-search optimizer is only
guaranteed to find the global optimum for all-V2 paths; also indicate expected
behavior for mixed or V3-only paths (e.g., return null, require an explicit
check that all Pool entries are V2 before using binary search, or fall back to
sampling/global search) and reference Pool and ArbOpportunity in the comment.
In `@src/dex/v3.zig`:
- Around line 495-546: The loop over ticks can stall if current_liquidity
becomes zero mid-iteration; add an early guard at the top of the for (ticks)
|tick_info| loop to break if current_liquidity == 0 (or if amount_remaining == 0
already handled) before calling computeSwapStep. Locate the loop that uses
computeSwapStep, check current_liquidity at its start and exit the loop
immediately when it's zero so computeSwapStep is never invoked with zero
liquidity and the loop cannot hang.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9efceaf7-ca50-4bca-b909-3e3ad996af63
📒 Files selected for processing (6)
docs/content/docs/dex-math.mdxsrc/dex/router.zigsrc/dex/v2.zigsrc/dex/v3.zigsrc/provider.zigsrc/uint256.zig
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/provider.zig`:
- Around line 471-479: The leak occurs because RPC error messages use an empty
string literal sentinel instead of a nullable/optional heap pointer, so
zero-length heap allocations (allocator.dupe(u8, "")) are skipped by the
`e.message.len > 0` check in freeBatchResults; change the message field's type
to an optional slice (e.g., ?[]u8 or ?[]const u8) where the sentinel is null for
non-allocated messages, update any places that construct the sentinel (use
.message = null) and places that duplicate/assign messages to set Some(buffer)
or null, and then modify freeBatchResults (in the .rpc_error arm) to free the
message only when it is not null (e.g., if (e.message)
allocator.free(e.message.*)). Ensure all callers and the
BatchCallResult/rpc_error constructor are updated to the new optional message
type.
- Around line 363-380: The defer currently frees all entries in bodies even if
some entries remain uninitialized on partial failure; change the
allocation/cleanup to track how many bodies were successfully created (e.g.,
maintain a local var like created_count or replace bodies with an ArrayList of
[]u8) and on error/free only iterate and free bodies[0..created_count], then
free the bodies container; ensure the same pattern is applied around params
(formatCallParams) and use the unique symbols bodies, ids, base_id,
provider.formatCallParams, HttpTransport.buildRequestBody, and
self.allocator.alloc to locate and update the loop so that only initialized
pointers are freed in the defer/cleanup path.
- Change RpcErrorData.message from []const u8 to ?[]u8 - Sentinel uses null (not string literal), eliminating @constcast - freeBatchResults frees message only when non-null - Initialize bodies array to empty slices before loop to prevent freeing uninitialized pointers on partial failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/provider.zig (1)
430-438: Avoid quadratic ID matching in the batch parser.Each response item scans the full
idsslice, so larger batches pay O(n²) matching cost before any decode work. Precomputing an ID→index lookup once would keep out-of-order matching cheap and make this path scale much better for MEV-sized batches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/provider.zig` around lines 430 - 438, The batch parser currently does a linear scan of the ids slice per response item (variables ids, idx, index), resulting in O(n²) behavior; instead build a one-time id→index lookup before iterating responses (e.g., a hashmap or array map keyed by id mapping to its usize index), populate it once from ids, and replace the per-item for-loop and idx assignment with a constant-time lookup of id to index inside the response loop; ensure the lookup handles missing ids the same way the current `index = idx orelse continue` does.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/provider.zig`:
- Around line 402-409: The code allocates `results` (BatchCallResult array) and
fills a sentinel value but doesn't free `results` if `std.json.parseFromSlice`
or later decoding fails, leaking memory; add an `errdefer
freeBatchResults(allocator, results);` immediately after the sentinel
initialization (right after the for-loop that sets each `r.* = .{ .rpc_error =
.{ .code = -1, .message = null } };`) so any early error path (e.g., the call to
`std.json.parseFromSlice` or subsequent validation) will free the partially
populated `results`; ensure you cancel or clear that errdefer when you take
ownership of `results` for normal success returns.
---
Nitpick comments:
In `@src/provider.zig`:
- Around line 430-438: The batch parser currently does a linear scan of the ids
slice per response item (variables ids, idx, index), resulting in O(n²)
behavior; instead build a one-time id→index lookup before iterating responses
(e.g., a hashmap or array map keyed by id mapping to its usize index), populate
it once from ids, and replace the per-item for-loop and idx assignment with a
constant-time lookup of id to index inside the response loop; ensure the lookup
handles missing ids the same way the current `index = idx orelse continue` does.
Prevents leaking the results array and any already-duped messages when JSON parsing or hex decoding fails mid-function.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BatchCallersends Neth_callrequests in a single JSON-RPC batch POST, with per-call error handling and out-of-order response matchingmulDivRoundingUpfor V3 fixed-point ceiling math,getAmountOutin uint256.zig now delegates to dex/v2.zigBenchmarks (Apple Silicon)
New files
src/dex/v2.zig-- V2 constant-product AMM (10 tests)src/dex/v3.zig-- V3 concentrated liquidity (21 tests)src/dex/router.zig-- Mixed V2/V3 routing (9 tests)docs/content/docs/batch-calls.mdx-- BatchCaller docsdocs/content/docs/dex-math.mdx-- DEX math docsCloses #11
Closes #16
Test plan
zig build test-- all existing + 46 new tests passzig fmt --check src/ bench/-- cleanzig build bench-u256-- benchmarks run, results aboveSummary by CodeRabbit
New Features
Documentation
Tests